Honor macOS title bar double-click setting - #76
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 74c120cf0c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
Changes requested
Reviewed head 74c120cf0c84f3513e6ff1cf0a7a7d3309347baf against base 42e92280dc6abb173b62d882df81d9f4400664e4. Required contract: honor macOS title-bar double-click preferences while preserving dragging, clickable header controls and non-macOS behavior.
P1: Gate the macOS action definitions so Linux and Windows pass their required native checks
src-tauri/src/lib.rs:20-33 adds TitleBarDoubleClickAction and title_bar_double_click_action unconditionally, but their only production use is inside #[cfg(target_os = "macos")]. Both existing non-macOS Clippy jobs therefore fail with dead_code promoted to errors. This is a PR-introduced cross-platform integration failure, not just a missing green badge: both native pipelines stop before their tests, and required CI fails.
Exact-head evidence: Linux job 104483479543 reports the unused enum at line 21 and function at line 27 under cargo clippy --workspace --locked --all-targets -- -D warnings; Windows job 104483479829 reports the same errors under the package Clippy command.
Smallest repair: gate both declarations with #[cfg(any(target_os = "macos", test))] or an equivalent narrow platform boundary so the pure tests still compile cross-platform. Keep warnings enforced. Exit criterion: both existing native jobs pass on the repaired head, including their currently skipped tests/tool integration.
Reviewed behavior and evidence limits
The frontend preserves primary/direct-background targeting and installs the replacement only on macOS desktop; web and non-macOS keep the existing drag-region attributes. The existing pointer-slop bot comment is not adopted as a new regression: lock-matched Tauri 2.11.5's previous window/scripts/drag.js already requires identical second-down/up coordinates. The sole PR commit contains a sign-off and hosted DCO Check succeeds, contrary to the older bot metadata comment.
Independent native review confirms the same platform-gating defect and narrow repair. The command re-reads the preference on each invocation, dispatches through the caller-injected window and keeps the Objective-C dependency macOS-only. Fresh/unset-preference OS behavior was not established in this source-only review; that uncertainty is not promoted into another blocker.
Existing CI run 34999321580 is completed/failure at this exact head. JavaScript, browser measurements and all Chromium/WebKit shards succeeded; Linux/Windows lint and the required aggregate failed as described above. Source/metadata review on the authorized laptop only: no checkout, installation, build, test execution, CI rerun or native-window acceptance. The author's separate local browser/PTY limitations are not the cause of these hosted failures.
Signed-off-by: klopez4212 <klopez4212@gmail.com>
Signed-off-by: klopez4212 <klopez4212@gmail.com>
74c120c to
22de810
Compare
|
🤖 Addressed the requested cross-platform gating on the rebased head. The title-bar action enum and preference mapper now compile only on macOS or in tests, preserving the pure tests without triggering dead-code errors on Linux/Windows. The command was also adapted to the newer generic Tauri runtime registry from main. Local evidence: focused title-bar tests passed, the native preference test passed, Clippy passed with warnings denied, 83 integration tests passed, 2,007 Vitest tests passed, 481/482 browser journeys passed in the full rerun, and the sole unrelated WebKit navigation timeout passed immediately in an isolated rerun. Both PR commits carry Signed-off-by trailers. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 22de81098c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Signed-off-by: klopez4212 <klopez4212@gmail.com>
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
Changes requested: one P2 remains
Reviewed head ad2897136814376d1de90ddb3cc85dfbe85388a9, current base ref 2387611dda87ed8d1259078263e7173b0d33a204, using the seven-file PR diff from merge-base 0beb523443557c189e5fcf16a3ac9e8437666690. Contract: honor the current macOS title-bar preference while preserving dragging, header controls and non-macOS behavior.
P2: Fill still dispatches native Zoom
The separate enum variants do not repair the native behavior. At src-tauri/src/lib.rs:58-64, Fill calls Tauri maximize/unmaximize. In the locked dependency chain, Tauri 2.11.5 forwards this through tauri-runtime-wry 2.11.4 to Tao 0.35.3 set_maximized. Tao's src/platform_impl/macos/util/async.rs:179-183 calls ns_window.zoom(None) for a titled, resizable window, which is the configuration used here. The new Zoom arm calls performZoom(None), which Apple documents as simulating the zoom button and then zooming the window.
Thus both preferences still select AppKit Zoom. Fill is specified to occupy the space between the menu bar and Dock, whereas Zoom toggles the standard/user frame; its default standard frame only nearly fills the screen. Selecting Fill and double-clicking the blank header therefore takes the Zoom path instead of implementing the requested Fill operation. This is verified by the dependency trace, not claimed as a native GUI reproduction.
Keep the repair narrow: dispatch Fill to an actual native fill/restore operation or equivalent current-display work-area sizing, rather than this Tauri maximize alias. Preserve native Zoom and the existing Minimize/None behavior. Exit criterion: demonstrate Fill versus Zoom and restoration in the built macOS app; the enum-mapping assertion alone cannot catch this defect.
Reconciled findings and validation
- The previous cross-platform
dead_codeblocker is repaired: both definitions now usecfg(any(target_os = "macos", test)). The command also fits the generic runtime registry. Pointer-slop and DCO findings are not repeated; the coordinate check matches locked Tauri behavior, and current commits have sign-offs. - Frontend/platform targeting, caller-injected window authority, command registration and macOS-only dependencies were traced. Independent native review reached the same Fill finding; the synchronous Wry/Tauri IPC path keeps the new AppKit call on the main thread.
- Exact-head CI passes automatic Linux/browser lanes, including 2,021 Vitest tests, the native preference mapper and Clippy. Windows native validation was skipped. This review executed no PR code, build or native GUI acceptance; unset-preference behavior remains an explicitly unverified case, not another blocker.
Signed-off-by: klopez4212 <klopez4212@gmail.com>
|
🤖 Fixed the remaining Fill/Zoom issue in f636e4a. Fill now saves the pre-Fill AppKit frame, animates to the current screen’s visible frame, and restores only when the window remains in that filled geometry; a moved, resized, zoomed, or display-changed window replaces stale restore state. Zoom still uses native performZoom, and Minimize/None are unchanged. Added regression coverage for Fill restoration and stale-frame replacement. Local evidence: focused title-bar tests passed, Clippy passed with warnings denied, production and design builds passed, 481/482 browser journeys passed in the broad run with the sole unrelated Chromium timeout passing on an exact retry (and passing in WebKit), and the built macOS app compiled/launched. One unrelated native terminal environment test remains blocked by the local shell’s ClickFix acknowledgement prompt, which was not bypassed. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f636e4aa0b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Signed-off-by: klopez4212 <klopez4212@gmail.com>
|
🤖 Follow-up CI repair pushed in 3d95334: the Fill frame map/state is now compiled only on macOS, while the pure transition helper remains available to cross-platform tests. This fixes Linux Clippy’s dead-code failure without changing runtime behavior. |
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
Re-review: no remaining code blockers
Reviewed head 3d953346fd977e107a0c32901a2c0caf190722bc, base ref 2387611dda87ed8d1259078263e7173b0d33a204, using the PR diff from merge-base 0beb523443557c189e5fcf16a3ac9e8437666690. This re-review concentrates on the Fill/restore repair since ad2897136814376d1de90ddb3cc85dfbe85388a9 and its integration with the already-reviewed shell contract.
Both previous blockers are resolved:
- Fill is no longer native Zoom.
src-tauri/src/lib.rs:120-168uses the current screen’svisibleFrameand AppKit frame sizing; native Zoom remainsperformZoom. Fill saves the previous frame, restores an unchanged filled window, and replaces saved state when geometry changes. - Platform gating is repaired. Runtime frame state and native dependencies are macOS-only; pure helpers remain testable on other platforms without the previous production dead-code failure.
Traced caller-window authority, command registration/state initialization, main-thread IPC execution, lock release before AppKit animation, and the single-window lifecycle. Direct-background/primary-button handling and web/non-macOS routing remain intact. Independent native review found no additional concrete defect. A proposed work-area-change finding was withdrawn: the existing restore-toggle policy is coherent, and a different policy was not an established requirement.
Validation and limits
CI run 35747086266, associated with this head, passes all automatic Linux/browser lanes: 2,021 Vitest tests, Rust/Clippy/tool integration including both title-bar tests, 476 browser journeys, and seven measurements. CI actually checked out synthetic merge 35187cb8b2a610d802aceec9b8e55d1751973621 (this head plus base e9717dda50eb25e81e0edad30b5d0500a052b591); it is not head-only or current-base native acceptance. Windows validation was skipped. Current DCO passes and all five PR commits have sign-offs.
This is a source/dependency and existing-CI review, not a macOS GUI acceptance claim. I did not execute a local build or test suite. The author reports a macOS build/launch, but actual Fill versus Zoom, restore, display transitions and animation re-entry still need native interaction evidence. Those limits are not independently established code defects.
My earlier code findings are resolved at this snapshot. This is a COMMENTED review, not approval.
Summary
Validation
origin/main